ref: Use Swift integrations #6862
Conversation
noahsmartin
force-pushed
the
swiftIntegrations
branch
2 times, most recently
from
ddc979e to
979af12
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6862 +/- ##
=============================================
+ Coverage 84.814% 84.845% +0.030%
=============================================
Files 453 453
Lines 27619 27628 +9
Branches 12110 12122 +12
=============================================
+ Hits 23425 23441 +16
- Misses 3908 4140 +232
+ Partials 286 47 -239
... and 40 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
noahsmartin
force-pushed
the
swiftIntegrations
branch
3 times, most recently
from
b0d6dfe to
4a01fe6
Compare
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 466e905 | 1223.78 ms | 1256.70 ms | 32.92 ms |
| 102cf89 | 1218.31 ms | 1239.78 ms | 21.47 ms |
| 7c7ac56 | 1225.90 ms | 1250.22 ms | 24.33 ms |
| 41c0aa8 | 1222.75 ms | 1262.40 ms | 39.65 ms |
| 73c9712 | 1238.57 ms | 1260.38 ms | 21.80 ms |
| 162cd7f | 1230.59 ms | 1256.76 ms | 26.16 ms |
| 52603c5 | 1229.35 ms | 1251.82 ms | 22.47 ms |
| f97a070 | 1218.88 ms | 1253.12 ms | 34.24 ms |
| 2a07609 | 1207.79 ms | 1233.77 ms | 25.98 ms |
| bbb9d5c | 1234.24 ms | 1261.76 ms | 27.52 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 466e905 | 23.75 KiB | 1010.51 KiB | 986.76 KiB |
| 102cf89 | 23.74 KiB | 891.02 KiB | 867.27 KiB |
| 7c7ac56 | 23.75 KiB | 902.49 KiB | 878.74 KiB |
| 41c0aa8 | 23.75 KiB | 986.80 KiB | 963.05 KiB |
| 73c9712 | 23.75 KiB | 908.01 KiB | 884.26 KiB |
| 162cd7f | 23.75 KiB | 908.39 KiB | 884.64 KiB |
| 52603c5 | 23.75 KiB | 969.66 KiB | 945.92 KiB |
| f97a070 | 23.75 KiB | 858.68 KiB | 834.93 KiB |
| 2a07609 | 23.75 KiB | 912.78 KiB | 889.03 KiB |
| bbb9d5c | 24.15 KiB | 1.01 MiB | 1014.91 KiB |
Previous results on branch: swiftIntegrations
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 9ef3a04 | 1217.35 ms | 1248.14 ms | 30.79 ms |
| f71365a | 1222.02 ms | 1248.70 ms | 26.68 ms |
| c1ceeee | 1218.08 ms | 1245.14 ms | 27.06 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 9ef3a04 | 24.14 KiB | 1.01 MiB | 1015.52 KiB |
| f71365a | 24.14 KiB | 1.01 MiB | 1015.45 KiB |
| c1ceeee | 24.14 KiB | 1.02 MiB | 1016.82 KiB |
noahsmartin
force-pushed
the
swiftIntegrations
branch
2 times, most recently
from
a35d333 to
10b2476
Compare
noahsmartin
requested review from
itaybre,
philipphofmann and
philprime
as code owners
noahsmartin
force-pushed
the
swiftIntegrations
branch
from
10b2476 to
4a2d481
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
noahsmartin
force-pushed
the
swiftIntegrations
branch
from
4a2d481 to
d0cf7ab
Compare
noahsmartin
force-pushed
the
swiftIntegrations
branch
from
d0cf7ab to
91e9878
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
Honestly, I'm unsure about this approach. I will have another look tomorrow, but here's what I already found.
| @property (nullable, nonatomic, strong) SentrySession *session; | ||
|
|
||
| @property (nonatomic, strong) NSMutableArray<id<SentryIntegrationProtocol>> *installedIntegrations; | ||
| @property (nonatomic, strong) NSMutableArray<id> *installedIntegrations; |
There was a problem hiding this comment.
m: That really feels wrong to me. Integrations still must adhere to SentryIntegrationProtocol.
| @property (nonatomic, strong) NSMutableArray<id> *installedIntegrations; | |
| @property (nonatomic, strong) NSMutableArray<id<SentryIntegrationProtocol>> *installedIntegrations; |
There was a problem hiding this comment.
With this change they wouldn't have to anymore, because now they could adhere to Sentry.Integration which doesn't have an ObjC representation. We could still make some other base class or protocol that is visible to Swift, but I don't think it would provide any value. I'll add another commit demonstrating how we could still do this so we can talk about it
There was a problem hiding this comment.
Good catch @philipphofmann there was one place where ObjC code still tried to dynamically call one of the methods on SentryIntegrationProtocol. So we can't quite get rid of the objc compatibility yet. I updated it to have a base protocol defined in ObjC that has the one function ObjC needs to know about (close) and now the Swift protocol also requires conformance to this objc protocol.
| let name: String | ||
|
|
||
| init<I: Integration>(_ integration: I.Type) where I.Dependencies == SentryDependencyContainer { | ||
| name = NSStringFromClass(I.self) |
There was a problem hiding this comment.
Bug: Generic Swift integration produces mangled name
Using NSStringFromClass on a generic Swift type like UserFeedbackIntegration<SentryDependencyContainer> returns a mangled, module-qualified name instead of a simple class name. This breaks trimmedInstalledIntegrationNames which expects names like "SentryUserFeedbackIntegration" but will receive something like "Sentry.UserFeedbackIntegration<Sentry.SentryDependencyContainer>". These trimmed names are sent to Sentry servers as part of SDK info, causing incorrect reporting of installed integrations.
Adds a new protocol for integrations that takes dependencies as an input. For the easiest migration conformers of the protocol can satisfy the
Dependenciesassociated type with justtypealias Dependencies = SentryDependencyContainer. This PR shows how it can be used to make the dependencies just define the minimum necessary dependencies so it can be easily mocked in tests. There are a few benefits to this over the current objc protocol:The protocol defines a failable initializer rather than a function returning a boolean. This means properties in the integration can be
letconstants rather than vars with default nil value. Making these immutable with help with a lot of swift properties like thread safety, and remove the need from handling null cases that should be unreachable.The associatedtype for dependencies decouples the SentryDependencyContainer from the integration, so each integration only defines the dependencies it needs and doesn't have a reliance on a particular type providing those dependencies.
No need for @objc or even NSObject in the integration classes
#skip-changelog
Closes #6863